fix: hierarchical dataflow simulator deadlock and HLS codegen (#561, #565)#577
fix: hierarchical dataflow simulator deadlock and HLS codegen (#561, #565)#577sunwookim028 wants to merge 4 commits intocornell-zhang:mainfrom
Conversation
build_dataflow_simulator only injected OMP parallel sections around the top-level function's func.call ops. Inner region kernel calls remained sequential, causing deadlock when kernels communicate via streams. Fix: extract OMP injection into _inject_omp_parallel_sections() helper and apply it recursively to every function with PE kernel calls, not just the top function. Addresses a part of cornell-zhang#561
- Added forward declarations for all functions in the HLS C++ emitter. - Ensured hierarchical regions are correctly marked with the dataflow attribute. - Fixed an issue where types were missing in function signatures by clearing the emitter's name table between passes.
- STATE.md: mark PR cornell-zhang#577 CI as PASSED, record stateful→Stateful and project structure changes in Completed This Cycle - ISSUE-003: update status to NEEDS-REVIEW (CI PASSED) - ISSUE-004: record alias removal (e665bbc) superseding temp alias (5595fab) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fangtangtang
left a comment
Fangtangtang
left a comment
There was a problem hiding this comment.
Thanks for the fixes! I just have some suggestions.
Also, could you add some tests to cover these fixes? It looks like they are addressing issues in hierarchical designs.
There was a problem hiding this comment.
does #557 still fail to fix the forward reference issue?
| if ctx.top_func is not None: | ||
| func_op.operation.move_before(ctx.top_func) |
There was a problem hiding this comment.
I guess this is also added to fix foward reference. Forward reference is valid in mlir but invalide in HLS backend. We'd better fix the issue in HLS backend codegen.
Could you please check whether existing fix (introduced in #557 ) already works?
| if isinstance(res.type, MemRefType) and len(res.type.shape) == 0: | ||
| op_ = ASTTransformer.build_scalar(ctx, arg) | ||
| arg_values.append(op_.result) |
There was a problem hiding this comment.
this would pass the scalar argument by value. Is this the intended behavior?
| # If it's a 0D memref (scalar), load it to get the value | ||
| if ( | ||
| isinstance(res.type, MemRefType) | ||
| and len(res.type.shape) == 0 | ||
| ): | ||
| op_ = ASTTransformer.build_scalar(ctx, arg) | ||
| arg_values.append(op_.result) |
There was a problem hiding this comment.
this would pass the scalar argument by value.
Also, I think args should only contain tensors (please correct me if I'm missing something). In that case, we should raise an error in the type infer phase if the user use a scalar here.
| if not hasattr(ctx, "global_op_cache"): | ||
| ctx.global_op_cache = {} |
There was a problem hiding this comment.
can we remove this since hasattr(ctx, "global_op_cache") is always True after the update in ASTContext
| if all_pe_calls_by_func is not None and pe_call_define_ops: | ||
| all_pe_calls_by_func[func_name] = pe_call_define_ops |
2 participants
Closes #561
Closes #565
Changes
Simulator recursive OMP (
allo/backend/simulator.py):build_dataflow_simulatoronly parallelized the top-level function. Inner-region kernel calls ran sequentially,
deadlocking when kernels communicate via streams. Fix: extracted
_inject_omp_parallel_sections()and applied it recursively to all functions with PEkernel calls.
global_op_cachemissing on copy (allo/ir/visitor.py):ASTContext.copy()didnot copy
global_op_cache, causingAttributeErrorin nested@df.kernelbodies thatuse
@statefulvariables.HLS codegen: forward declarations + dataflow pragma (
allo/ir/builder.py,mlir/lib/Translation/EmitVivadoHLS.cpp): the emitter emitted inner kernel functionsafter the wrapper that calls them, producing "undeclared identifier" errors in Vitis HLS.
Fix: added a forward-declaration pass before the full emission pass. Also ensures nested
df.regionbodies receive#pragma HLS dataflow.nameTableis cleared between passesso type names are re-emitted correctly.
Note: cherry-picked onto current main after [Fix] several issues related to
tests/dataflow/test_hierachical.py#557'semitFunctionSignaturerefactor;conflict resolved by preserving [Fix] several issues related to
tests/dataflow/test_hierachical.py#557'sSmallVector<Value,8>return type whilegrafting in the forward-declaration loop and
nameTable.clear().Tests
tests/dataflow/test_hierachical.py— simulator: PASSED